chore(contexts): Remove default-trace-ID option by thetruecpaul · Pull Request #5979 · getsentry/relay

thetruecpaul · 2026-05-11T22:08:55Z

Shipped this a week ago and have seen no problems. 👍

Dav1dde

Thanks, for coming back and cleaning this up.

You accidentally updated/reverted the submodules, sentry-conventions and sentry-native. You might be missing a git submodule update --recursive --init.

shashjar · 2026-05-12T21:10:08Z

+                .0
+                .is_none()
+        {
            contexts.add(TraceContext::random(event_id))


If there's a TraceContext with valid information but it's missing trace_id (not sure if this actually occurs in practice) - I think we'll end up overriding the entire context with a new one that has only trace_id? I think we want to get_or_default the trace context, then check whether trace_context.trace_id is null, and then set only trace_context.trace_id if so

shashjar · 2026-05-12T21:10:25Z

        // If the event lacks a TraceContext, add a random one.

-        if config.derive_trace_id && !contexts.contains::<TraceContext>() {
+        if !contexts.contains::<TraceContext>()


Can we add unit tests for this logic somewhere?

Updating the tests is what I'm working on now! :-)

Just a drive by comment before bed, an integration test (if it doesn't already exist) would be great, we do have a decent framework for them and they work great for regression tests.

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 9942b0b. Configure here.}

sentry · 2026-05-18T20:10:23Z

+            let trace_ctx = contexts.get_or_default::<TraceContext>();
+            if trace_ctx.trace_id.0.is_none() {
+                trace_ctx.trace_id = Annotated::new(TraceId::from(event_id))
+            }
        }


Bug: If an event has an existing but empty TraceContext, the normalization logic only sets the trace_id, leaving the required span_id unset, creating an inconsistent state.
_{Severity: MEDIUM}

Suggested Fix

In the else branch where contexts.contains::<TraceContext>() is true, ensure that span_id is also set along with trace_id. This could be done by generating a new span_id if trace_ctx.span_id is None, similar to how trace_id is handled. Alternatively, consider replacing the partial context with a fully new one using TraceContext::random() if the existing one is incomplete.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: relay-event-normalization/src/event.rs#L1370-L1374 Potential issue: When an event is processed that already contains a `TraceContext` but lacks a `trace_id` (e.g., `{"contexts": {"trace": {}}}`), the normalization logic enters a branch that sets the `trace_id`. However, this logic fails to also generate and set the `span_id`. This is inconsistent with the case where no `TraceContext` exists, in which a new context is created with both a `trace_id` and a `span_id` via `TraceContext::random()`. The result is a partially normalized trace context where the `span_id`, a required field, remains `None`.

thetruecpaul · 2026-05-18T20:26:37Z

Adding the skip-changelog label for the Python changelog since it's just updating a couple tests

shashjar · 2026-05-18T20:43:17Z

+        assert set(event["contexts"].keys()) == {"browser", "client_os", "trace"}
    else:
-        assert "contexts" not in event
+        assert set(event["contexts"].keys()) == {"trace"}


Should we assert the fields contained in the trace context, e.g. trace_id or other necessary fields are present?

Generally a good idea, but these are the Python library tests and this is a test for normalizing the user agent, so I think this is fine.

shashjar · 2026-05-18T21:00:02Z

        // If the event lacks a TraceContext, add a random one.

-        if config.derive_trace_id && !contexts.contains::<TraceContext>() {
+        if !contexts.contains::<TraceContext>() {


I think you could collapse this if/else with something like the below (does span_id default have to be added as well?)

let trace_ctx = contexts.get_or_default::<TraceContext>(); if trace_ctx.trace_id.0.is_none() { trace_ctx.trace_id = Annotated::new(TraceId::from(event_id)) }

So, there are four possible cases:

The TraceContext is not set. In this case, we generate a random SpanID and a TraceID from the EventID. (See TraceContext::random.)

The TraceContext is set with a TraceID and SpanID. In this case, we want to do nothing.

The TraceContext is set with no TraceID but an existing SpanID. In this case, we want to create new TraceID but not change the existing SpanID.

The TraceContext is set with no TraceID and no SpanID. In this case, we are currently only generating the TraceID. My reasoning is that "empty context" is different from "missing context" and we should only make the minimal update needed for our purposes (ingesting to EAP), but I could be talked around to updating both for the purpose of consistency with case (1).

All this to say — what does default mean for TraceContext? Is it something with all the attributes set to None? Ideally we'd want to short-circuit to TraceContext::random if the context doesn't exist, not TraceContext::default or whatnot (this method doesn't exist).

I would say we should defer to @Dav1dde on the question of default span ID (whether or not that should be added, and where that should be added, or if it is already).

But I think the code I attached above works for all of the cases in which trace ID needs to be populated (and additional if branches can be added if needed if there are other defaults we need to set)

The more I look into this I begin to question whether this doesn't have a lot of unintentional side-effects.
We're generating a trace context here for the purpose of storing in EAP, but that context is also visible to users with side effects as running it through dynamic sampling to mark the sampling decision on the error.
That's more a fundamental concern which also was brought up with the introduction of the feature flag.

I would say we should defer to @Dav1dde on the question of default span ID (whether or not that should be added, and where that should be added, or if it is already).

The span_id is required on the context, that means if you want to generate a context it also needs to be filled with a span_id.

All this to say — what does default mean for TraceContext? Is it something with all the attributes set to None?

All attributes set to Annotated::empty() (practically None). There is an argument to be made, that it should not implement Default at all.

Dav1dde

Looking through it, I noticed some bigger problems. This is actually right now applied to transactions, which makes an existing validation of a trace context being required order dependent, in the worst case it will invent trace or span ids for transactions.

Some of the metadata stuff is inconsistent, with no context metadata is added, with a partial context it is not (presumably because there is this assumption that the schema processor would add it?).
I think we should just not bother with adding metadata to the individual fields when the context is created. If a metadata is desired, we should instead attach it to the context element which was created.

I really don't like that we have to touch a lot of unrelated tests, so I think we should keep the normalization option and that is also a great place to add some documentation why we're doing this.

Relay's normalization is complex enough that I made changes and notes during the review, ended up polishing it and making it a PR with the changes I describe here: #6009

Dav1dde · 2026-05-20T09:24:35Z

+        assert set(event["contexts"].keys()) == {"browser", "client_os", "trace"}
    else:
-        assert "contexts" not in event
+        assert set(event["contexts"].keys()) == {"trace"}


Generally a good idea, but these are the Python library tests and this is a test for normalizing the user agent, so I think this is fine.

Dav1dde · 2026-05-20T09:28:52Z

This is another presumably unintentional submodule revert

Dav1dde · 2026-05-20T09:36:24Z

        // If the event lacks a TraceContext, add a random one.

-        if config.derive_trace_id && !contexts.contains::<TraceContext>() {
+        if !contexts.contains::<TraceContext>() {


The more I look into this I begin to question whether this doesn't have a lot of unintentional side-effects.
We're generating a trace context here for the purpose of storing in EAP, but that context is also visible to users with side effects as running it through dynamic sampling to mark the sampling decision on the error.
That's more a fundamental concern which also was brought up with the introduction of the feature flag.

I would say we should defer to @Dav1dde on the question of default span ID (whether or not that should be added, and where that should be added, or if it is already).

The span_id is required on the context, that means if you want to generate a context it also needs to be filled with a span_id.

All this to say — what does default mean for TraceContext? Is it something with all the attributes set to None?

All attributes set to Annotated::empty() (practically None). There is an argument to be made, that it should not implement Default at all.

Dav1dde · 2026-05-20T09:49:06Z

+        } else {
+            let trace_ctx = contexts.get_or_default::<TraceContext>();
+            if trace_ctx.trace_id.0.is_none() {
+                trace_ctx.trace_id = Annotated::new(TraceId::from(event_id))


Isn't this inconsistent with TraceContext::random which does add metadata for a "missing" trace id?

Dav1dde · 2026-05-20T09:58:40Z

-    /// Should add a random trace ID to events that lack one.
-    pub derive_trace_id: bool,


I think we should keep this here, add some documentation why this is needed and then just permanently set it to true where the config gets created.

Usually I'd agree with just by default enabling it, but there is a large impact on tests which are testing unrelated things but now need to assert a random trace id and context.

We can still test the context creation with the boolean and have integration tests to ensure this actually is enabled.

Dav1dde · 2026-05-20T11:28:30Z


-        if config.derive_trace_id && !contexts.contains::<TraceContext>() {
+        if !contexts.contains::<TraceContext>() {
            contexts.add(TraceContext::random(event_id))


TraceContext::random interior should not add metadata to the individual fields that they were missing (trace_id/span_id).

If you want to make the trace context effectively required, instead we should add this marker to the context itself.

thetruecpaul requested a review from a team May 11, 2026 22:08

thetruecpaul requested a review from a team as a code owner May 11, 2026 22:08

thetruecpaul force-pushed the cpaul/051126/ship-default-trace-id branch from 04f1690 to a55366d Compare May 11, 2026 22:12

shashjar approved these changes May 11, 2026

View reviewed changes

Comment thread relay-event-normalization/src/event.rs

Dav1dde requested changes May 12, 2026

View reviewed changes

Comment thread relay-event-normalization/src/event.rs

Comment thread CHANGELOG.md Outdated

thetruecpaul force-pushed the cpaul/051126/ship-default-trace-id branch 2 times, most recently from 274748d to 4654bdc Compare May 12, 2026 17:50

cursor Bot reviewed May 12, 2026

View reviewed changes

Comment thread relay-event-normalization/src/event.rs

thetruecpaul force-pushed the cpaul/051126/ship-default-trace-id branch from 4654bdc to de252d4 Compare May 12, 2026 19:38

sentry Bot reviewed May 12, 2026

View reviewed changes

Comment thread relay-event-normalization/src/event.rs Outdated

cursor Bot reviewed May 12, 2026

View reviewed changes

Comment thread relay-event-normalization/src/event.rs Outdated

thetruecpaul commented May 12, 2026

View reviewed changes

Comment thread relay-event-normalization/src/event.rs Outdated

shashjar reviewed May 12, 2026

View reviewed changes

thetruecpaul force-pushed the cpaul/051126/ship-default-trace-id branch from de252d4 to 9942b0b Compare May 12, 2026 23:27

cursor Bot reviewed May 12, 2026

View reviewed changes

Comment thread relay-event-normalization/src/event.rs

Dav1dde reviewed May 13, 2026

View reviewed changes

Comment thread relay-event-normalization/src/event.rs

chore(contexts): Remove default-trace-ID option

84c16a3

thetruecpaul force-pushed the cpaul/051126/ship-default-trace-id branch from 9942b0b to e4b0067 Compare May 18, 2026 20:05

sentry Bot reviewed May 18, 2026

View reviewed changes

thetruecpaul requested a review from Dav1dde May 18, 2026 20:14

thetruecpaul force-pushed the cpaul/051126/ship-default-trace-id branch from e4b0067 to f98e9c9 Compare May 18, 2026 20:18

thetruecpaul added the skip-changelog label May 18, 2026

tests

c024935

thetruecpaul force-pushed the cpaul/051126/ship-default-trace-id branch from f98e9c9 to c024935 Compare May 18, 2026 20:27

thetruecpaul requested a review from a team May 18, 2026 20:37

shashjar reviewed May 18, 2026

View reviewed changes

Dav1dde mentioned this pull request May 20, 2026

ref(errors): Unconditionally create a trace context #6009

Open

Dav1dde requested changes May 20, 2026

View reviewed changes

		/// Should add a random trace ID to events that lack one.
		pub derive_trace_id: bool,

Conversation

thetruecpaul commented May 11, 2026

Uh oh!

Uh oh!

Dav1dde left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shashjar May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sentry Bot May 18, 2026

Choose a reason for hiding this comment

Uh oh!

thetruecpaul commented May 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dav1dde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shashjar May 12, 2026 •

edited

Loading